Code Review

Overall Assessment

Good implementation that addresses the issue requirements for dynamic time estimates. The code follows most project conventions and patterns. There are a few areas that need attention.

✅ Strengths

1. Clean extension function: The new Activity.timestamp() extension in Activities.kt:74-80 properly handles both Lightning and Onchain activities with correct fallback logic for confirmTimestamp.

2. Consistent pattern usage: The code follows existing patterns for accessing ViewModels via Compose locals (blocktankViewModel) and StateFlow collection.

3. Code reuse: Good use of the new Activity.timestamp() extension to replace duplicated timestamp logic in both ActivityRow.kt and ActivityDetailScreen.kt.

4. Composable function placement: FeeRate.getFeeDescription() is appropriately marked as @Composable since it uses stringResource().

⚠️ Issues & Recommendations

1. Null Safety Concern (Medium Priority)

Location: ActivityRow.kt:66-69, ActivityDetailScreen.kt:195-198

val blocktankInfo by blocktankViewModel?.info?.collectAsStateWithLifecycle() ?: remember {
    mutableStateOf(null)
}
val feeRates = blocktankInfo?.onchain?.feeRates

Issue: When feeRates is null, FeeRate.getFeeDescription() falls back to NORMAL, which returns "±20m". This might not reflect the actual transaction confirmation time if fee rate data is unavailable.

Recommendation: Consider handling the null case more explicitly:

• Add a loading state while fee rates are being fetched
• Or display a generic message like "Pending" when fee rates aren't available
• Log a warning when falling back to NORMAL

2. TODO Comments Should Be Removed or Tracked (Low Priority)

Location: ActivityRow.kt:326, ActivityDetailScreen.kt:964

// TODO remove this method after transifex update
private fun String.removeEstimationSymbol() = this.replace("±", "")

Issue: These TODO comments indicate temporary workarounds but lack:

• A timeline or issue reference
• Explanation of what "transifex update" means
• Why the ± symbol needs to be removed

Recommendation:

• Create a follow-up issue to track the transifex update and reference it in the comment
• Or if the strings are already updated in Transifex, remove the workaround now
• Add more context: // TODO(#ISSUE_NUMBER): Remove after transifex updates string resources to not include ± symbol

3. Missing Unit Tests (Medium Priority)

Issue: No unit tests were added for:

• Activity.timestamp() extension function
• FeeRate.getFeeDescription() and FeeRate.fromSatsPerVByte() logic
• The new time estimation logic

Recommendation: Add unit tests to verify:

// Test cases for Activities.kt
- timestamp() returns v1.timestamp for Lightning activities
- timestamp() returns v1.confirmTimestamp when confirmed = true and confirmTimestamp != null
- timestamp() falls back to v1.timestamp when confirmed = true but confirmTimestamp = null
- timestamp() returns v1.timestamp when confirmed = false

// Test cases for FeeRate.kt
- fromSatsPerVByte() correctly maps fee rates to enum values
- getFeeDescription() returns correct string for each fee rate tier
- getFeeDescription() handles null feeEstimates correctly

4. Potential String Resource Issue (Low Priority)

Location: String replacements in ActivityRow.kt:132,140 and ActivityDetailScreen.kt:745

.replace("{duration}", duration.removeEstimationSymbol())

Issue: Using .replace() for string formatting is fragile and doesn't support localization placeholders properly.

Recommendation: Consider using Android's string format arguments:

<string name="wallet__activity_transfer_spending_pending">From Savings (%1$s)</string>

stringResource(R.string.wallet__activity_transfer_spending_pending, duration)

This would be more robust and align with Android best practices.

5. Code Comment Missing (Low Priority)

Location: Activities.kt:74

The new Activity.timestamp() function would benefit from a KDoc comment explaining:

• When confirmTimestamp vs timestamp is used
• Why we fall back to timestamp when confirmTimestamp is null

🔒 Security

No security concerns identified.

⚡ Performance

No performance concerns. The changes are minimal and don't introduce any expensive operations.

📋 Code Style & Conventions

Follows CLAUDE.md guidelines:

• ✅ Uses extension functions appropriately
• ✅ Preserves string resources
• ✅ Uses @Composable annotation correctly
• ✅ Places modifiers in proper position
• ✅ Uses StateFlow with collectAsStateWithLifecycle()

Minor style note: The new extension function Activity.timestamp() is placed in a good location in Activities.kt, maintaining alphabetical/logical ordering with other Activity extensions.

🎯 Suggested Next Steps

1. Add unit tests for the new timestamp() extension and FeeRate methods
2. Clarify or resolve the TODO comments about transifex
3. Consider more explicit handling of null feeRates case
4. (Optional) Refactor string replacements to use proper format arguments

✅ Approval Recommendation

Approved with minor suggestions. The code correctly implements the required functionality and follows project patterns. The concerns raised are mostly about test coverage and code maintenance, not blocking issues. Consider addressing the recommendations in a follow-up PR if not in this one.